Conversation
WalkthroughMain now listens for OS signals in a loop, handling Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/examples/restart-on-config-change/README.md (1)
16-34: Script no longer watches for multiple config changes after switching to SIGUSR1With
kill -SIGUSR1 $PIDthe llama-swap process keeps running, so the subsequentwait $PIDblocks until the process eventually exits. That means:
- The first config change triggers a reload via SIGUSR1.
- Further changes are not detected because the loop doesn’t reach the next
inotifywaituntil the process dies.This is at odds with the description that the script “can be used to watch
config.yamlfor changes and send theSIGUSR1signal … when it detects a change.”Consider restructuring so the process is started once and the loop only waits on inotify + sends SIGUSR1 while the PID is alive, for example:
-while true; do - # Start the process again - ./llama-swap-linux-amd64 -config $1 -listen :1867 & - PID=$! - echo "Started llama-swap with PID $PID" - - # Wait for modifications in the specified directory or file - inotifywait -e modify "$1" - - # Check if process exists before sending signal - if kill -0 $PID 2>/dev/null; then - echo "Sending SIGUSR1 to $PID" - kill -SIGUSR1 $PID - wait $PID - else - echo "Process $PID no longer exists" - fi - sleep 1 -done +# Start the process once +./llama-swap-linux-amd64 -config "$1" -listen :1867 & +PID=$! +echo "Started llama-swap with PID $PID" + +while kill -0 "$PID" 2>/dev/null; do + # Wait for modifications in the specified file + inotifywait -e modify "$1" + + if kill -0 "$PID" 2>/dev/null; then + echo "Sending SIGUSR1 to $PID" + kill -SIGUSR1 "$PID" + else + echo "Process $PID no longer exists" + break + fi +done + +wait "$PID"
🧹 Nitpick comments (2)
docs/configuration.md (1)
387-397: Hot‑reload section is clear and consistent with implementationThe SIGUSR1 instructions and Docker example match the new signal handling behavior and the linked example README; this looks good. If you expect many Windows users, consider a brief note that SIGUSR1 is POSIX‑only and that Windows should rely on
-watch-configor other mechanisms.llama-swap.go (1)
80-81: SIGUSR1 handling and signal loop look good; check Windows build and reload concurrencyThe refactored signal loop cleanly separates shutdown (SIGINT/SIGTERM) from reload (SIGUSR1), and the logic matches the docs and example output. A couple of follow‑ups to consider:
- Cross‑platform build: This file has no build tags in the snippet. If it’s compiled on Windows as well,
syscall.SIGUSR1may not be defined there, which would break Windows builds. If that’s the case, either:
- Add an appropriate build tag (e.g.
//go:build !windows) to this file, or- Guard SIGUSR1 registration behind a platform check / separate Unix‑only file.
- Reload serialization (optional):
reloadProxyManagercan now be triggered both via fsnotify (-watch-config) and SIGUSR1. If those are ever used together, you could see concurrent reload calls mutatingsrv.Handlerandconf. If that scenario is realistic for your users, consider serializing reloads (e.g. via a mutex or a single “reload” goroutine) to avoid subtle races.Please verify that:
- Cross‑platform builds (especially
GOOS=windows) still compile successfully withsyscall.SIGUSR1in this file.- You’re comfortable with the current concurrency story for
reloadProxyManageror want to guard it with a mutex.Also applies to: 173-201
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/configuration.md(1 hunks)docs/examples/restart-on-config-change/README.md(3 hunks)llama-swap.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-05T22:57:53.793Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 155
File: proxy/process.go:359-372
Timestamp: 2025-06-05T22:57:53.793Z
Learning: In Go, cmdWaitChan in proxy/process.go is a signal channel (chan struct{}) that follows the pattern where waitForCmd() closes the channel when cmd.Wait() returns, and stopCommand() waits on it. Reading from a closed channel returns immediately, so <-cmdWaitChan won't block indefinitely.
Applied to files:
llama-swap.go
📚 Learning: 2025-10-15T12:15:22.706Z
Learnt from: tesurijp
Repo: mostlygeek/llama-swap PR: 350
File: llama-swap-windows.go:17-17
Timestamp: 2025-10-15T12:15:22.706Z
Learning: In the llama-swap project, the `listenStr` flag format includes the colon prefix (e.g., `:8080`), so when constructing URLs, it can be directly concatenated with `http://localhost` to produce the correct format like `http://localhost:8080`.
Applied to files:
docs/examples/restart-on-config-change/README.md
🧬 Code graph analysis (1)
llama-swap.go (1)
proxy/proxymanager.go (1)
ProxyManager(30-53)
🪛 LanguageTool
docs/examples/restart-on-config-change/README.md
[grammar] ~3-~3: Ensure spelling is correct
Context: ...he configuration file can take a bit of trail and error to get a model configuration ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (1)
docs/examples/restart-on-config-change/README.md (1)
39-48: Updated sample output matches new reload behaviorThe example log sequence (SIGUSR1 received, “Reloading configuration…”, “Configuration Changed”, “Configuration Reloaded”) aligns with the new signal handling and
reloadProxyManagermessages inllama-swap.go.
f4ff658 to
169b3fb
Compare
|
Nice idea, would it make more sense to re-appropriate |
|
@ryan-steed-usa I first started with SIGHUP as a quick patch but the docker kill command strangely terminated the container. Let me try again maybe I overlooked something. EDIT: I changed it to |
169b3fb to
b38cfcb
Compare
b38cfcb to
c9e0dfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llama-swap.go (1)
88-119: Data race on srv.Handler during concurrent reload.The
srv.Handlerfield is accessed concurrently without synchronization:
- Write access: Lines 100 and 117 assign a new ProxyManager to
srv.Handler- Read access: The HTTP server reads
srv.Handler.ServeHTTP()for each incoming requestIn Go, unsynchronized concurrent read/write access to the same memory location constitutes a data race, even for pointer assignments. This can lead to undefined behavior, crashes, or serving requests with a partially-initialized handler.
Recommended solutions:
- Wrap srv.Handler in a synchronized wrapper (preferred for minimal disruption):
// Add near top of main(): type SafeHandler struct { mu sync.RWMutex handler http.Handler } func (sh *SafeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { sh.mu.RLock() h := sh.handler sh.mu.RUnlock() h.ServeHTTP(w, r) } func (sh *SafeHandler) Set(h http.Handler) { sh.mu.Lock() sh.handler = h sh.mu.Unlock() }Then modify the server setup and reloadProxyManager:
-srv := &http.Server{ - Addr: *listenStr, -} +safeHandler := &SafeHandler{} +srv := &http.Server{ + Addr: *listenStr, + Handler: safeHandler, +}And in reloadProxyManager:
- if currentPM, ok := srv.Handler.(*proxy.ProxyManager); ok { + if sh, ok := srv.Handler.(*SafeHandler); ok { + sh.mu.RLock() + currentPM, isPM := sh.handler.(*proxy.ProxyManager) + sh.mu.RUnlock() + if isPM { // ... reload logic ... - srv.Handler = newPM + sh.Set(newPM) + } } else { // ... initial load ... - srv.Handler = newPM + if sh, ok := srv.Handler.(*SafeHandler); ok { + sh.Set(newPM) + } }
- Use atomic.Value for lock-free reads (alternative):
var handler atomic.Value // stores http.Handler srv.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { handler.Load().(http.Handler).ServeHTTP(w, r) }) // In reloadProxyManager: handler.Store(newPM)
♻️ Duplicate comments (1)
docs/examples/restart-on-config-change/README.md (1)
3-3: Typo fixed: "trial and error" is now correct.This addresses the previous review feedback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/configuration.md(1 hunks)docs/examples/restart-on-config-change/README.md(2 hunks)llama-swap.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-05T22:57:53.793Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 155
File: proxy/process.go:359-372
Timestamp: 2025-06-05T22:57:53.793Z
Learning: In Go, cmdWaitChan in proxy/process.go is a signal channel (chan struct{}) that follows the pattern where waitForCmd() closes the channel when cmd.Wait() returns, and stopCommand() waits on it. Reading from a closed channel returns immediately, so <-cmdWaitChan won't block indefinitely.
Applied to files:
llama-swap.go
🧬 Code graph analysis (1)
llama-swap.go (1)
proxy/proxymanager.go (1)
ProxyManager(30-53)
🔇 Additional comments (2)
llama-swap.go (2)
80-80: LGTM: SIGHUP added to signal notifications.The signal is correctly registered for handling configuration reloads.
173-201: Signal handling loop correctly implements reload and shutdown.The refactored signal handling properly distinguishes between:
- Graceful shutdown on SIGINT/SIGTERM (with timeout and cleanup)
- Hot reload on SIGHUP
- Logging for unexpected signals
The loop structure ensures the goroutine exits after shutdown signals but continues processing after reload signals.
| ... | ||
| Sending SIGHUP to 495455 | ||
| Received SIGHUP. Reloading configuration... | ||
| Configuration Changed | ||
| Configuration Reloaded | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
Output example shows correct SIGHUP flow but won't match current script.
The example output correctly demonstrates the SIGHUP reload sequence:
- "Sending SIGHUP to [PID]"
- "Received SIGHUP. Reloading configuration..."
- "Configuration Changed"
- "Configuration Reloaded"
However, this output won't occur with the current script because it sends SIGUSR1 instead of SIGHUP (see comment on lines 34-35). After fixing the signal in line 35, this example will be accurate.
🤖 Prompt for AI Agents
docs/examples/restart-on-config-change/README.md lines 52-58: the example output
shows a SIGHUP-based reload sequence but the actual example script sends SIGUSR1
(see comment/usage around line 34-35), so update the script to send SIGHUP
instead of SIGUSR1 (change the signal argument at line 35 to "SIGHUP"), then
re-run the example to ensure the README output matches runtime behavior.
|
@blob42 I've tested this with Podman (via my forked franken-builds) and it works well. Thanks! |
|
I still believe that this PR introduces useful functionality but it has helped me to understand why Docker/Podman with |
|
The
It is a problem with the Perhaps instead of listening for new signals a |
|
Overriding the entrypoint isn't necessary either since we already use the exec form of ENTRYPOINT:
So a command like this works by appending the arguments: docker run -v /path/to/config_dir:/config llama-swap:cpu -config /config/config.yaml -watch-configIt would help if this were to be mentioned in the docs. |
|
@ryan-steed-usa thanks. I forgot that with go’s flags package defining I’ll be sure to update the docs as mounting a config directory is the workaround for making —watch-config work reliably. |
- Allows quick reloading with SIGHUP without when filesytem watch is not available like on a default docker setup `docker kill -s SIGHUP` - Also refactored the signal handling in a single logical unit Signed-off-by: blob42 <contact@blob42.xyz>
c9e0dfb to
c7ca85e
Compare
|
@mostlygeek I am not sure If you still plan to merge this PR but I just made a rebase in case you plan to merge it |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/examples/restart-on-config-change/README.md (1)
12-13:⚠️ Potential issue | 🔴 CriticalCritical: reload signal is still wrong (
SIGUSR1instead ofSIGHUP).At Line 35, the script sends
SIGUSR1, but the app reload path is wired toSIGHUP. So config reload won’t trigger. This also makes the guidance at Lines 12-13 and sample output at Lines 53-56 inconsistent with executable behavior.Proposed fix
-# For docker users, consider replacing: -# `kill -USR1 $PID` with `docker kill -s SIGHUP container_name` +# For docker users, equivalent command: +# `docker kill -s SIGHUP container_name` @@ - kill -USR1 $PID + kill -HUP $PIDAlso applies to: 34-35, 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples/restart-on-config-change/README.md` around lines 12 - 13, The README's example uses the wrong signal (SIGUSR1) while the app listens for SIGHUP, so update all occurrences of SIGUSR1 to SIGHUP (including the docker guidance, the script example around the reload command, and the sample output lines) to make the documented command, the advice (`kill -SIGHUP` / `docker kill -s SIGHUP`), and the shown output consistent with the app's reload path; ensure every mention (e.g., the script send signal, the docker note, and the sample output) is changed to SIGHUP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/examples/restart-on-config-change/README.md`:
- Around line 12-13: The README's example uses the wrong signal (SIGUSR1) while
the app listens for SIGHUP, so update all occurrences of SIGUSR1 to SIGHUP
(including the docker guidance, the script example around the reload command,
and the sample output lines) to make the documented command, the advice (`kill
-SIGHUP` / `docker kill -s SIGHUP`), and the shown output consistent with the
app's reload path; ensure every mention (e.g., the script send signal, the
docker note, and the sample output) is changed to SIGHUP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e519d971-8c0a-44cf-a90c-c2cb8ac57f8f
📒 Files selected for processing (3)
docs/configuration.mddocs/examples/restart-on-config-change/README.mdllama-swap.go
✅ Files skipped from review due to trivial changes (1)
- docs/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- llama-swap.go
not available like on a default docker setup
docker kill -s SIGHUP